Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

examples/leds_shell: add example for interactive LEDs/GPIO control #20782

Merged
merged 4 commits into from
Jul 18, 2024
Merged

examples/leds_shell: add example for interactive LEDs/GPIO control #20782

merged 4 commits into from
Jul 18, 2024

Conversation

krzysztof-cabaj
Copy link
Contributor

Contribution description

Rationale: When I remember my first steps as RIOT newbie, I would like as soon as possible flash the program and
show that it works and could control something, for example LED. However, for RIOT newbie this is not so easy. Of course there is examples/blink - but it works automatically. Manual control is something, in my humble opinion, missing.

This simple program allows easy control of internal LEDs via RIOT CLI. Additionally, it allows initialization of any GPIO port in output mode and manually set HIGH/LOW state.

From the programming learning perspective, analysis and experiments with this program could teach:

  • RIOT CLI,
  • Adding own functions,
  • Basic GPIO.

Any comments are welcome.

Testing procedure

Flash examples/LEDs and check if number of internal LEDs is properly detected.

I tested it with boards from 1 to 4 LEDs, respectively: nucleo-l496rg, stm32l47g-disco/native', 'nucleo-f207zg and stm32f469i-disco.

Read README.md and check if everything is clear.

Issues/PRs references

PR #20637

@github-actions github-actions bot added Area: doc Area: Documentation Area: examples Area: Example Applications labels Jul 10, 2024
mguetschow
mguetschow reviewed Jul 10, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, just had a brief look at it for now.

Although this is somehow opposed to your intended use-case, aren't there some default shell commands already for this kind of GPIO/LED control?

examples/LEDs/main.c Outdated Show resolved Hide resolved
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 10, 2024
@riot-ci
Copy link

riot-ci commented Jul 10, 2024 •
edited
Loading

Murdock results

✔️ PASSED

9959e62 examples/leds_shell: code refactoring-one led and gpio commands

Success Failures Total Runtime
18 0 19 01m:21s

Artifacts

@dylad
Copy link
Member

dylad commented Jul 10, 2024

Maybe we can come up with a better name ? I find it a bit disturbing to have a tests/leds and examples/LEDs.

@krzysztof-cabaj
Copy link
Contributor Author

Maybe we can come up with a better name ? I find it a bit disturbing to have a tests/leds and examples/LEDs.

What do you think about examples/leds_cli or examples/cli_leds?

@krzysztof-cabaj
Copy link
Contributor Author

Although this is somehow opposed to your intended use-case, aren't there some default shell commands already for this kind of GPIO/LED control?

During my few years with RIOT I did not find any such commands. From similar programs, I only knows examples/blinky and mentioned by @dylad tests/leds but they lacks interactivity.

@mguetschow mguetschow mentioned this pull request Jul 11, 2024
Merged
@krzysztof-cabaj krzysztof-cabaj changed the title examples/LEDs: add example for interactive LEDs/GPIO control examples/leds_shell: add example for interactive LEDs/GPIO control Jul 12, 2024
@krzysztof-cabaj krzysztof-cabaj force-pushed the example-LEDs branch 3 times, most recently from f3af37c to fadfe6d Compare July 12, 2024 20:56
@krzysztof-cabaj
Copy link
Contributor Author

As @dylad suggested I change the name of application to leds_shell - I hope that current name will be clear for everybody.

The code is refactored using inline functions added by @mguetschow in the PR #20783.
The code is know simpler and looks better.

@krzysztof-cabaj krzysztof-cabaj force-pushed the example-LEDs branch 2 times, most recently from db70901 to ef0ec52 Compare July 12, 2024 21:15
mguetschow
mguetschow requested changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adapting your code, looks much cleaner now! I've got comments mostly on the Makefile and README below.

examples/leds_shell/Makefile Outdated Show resolved Hide resolved
examples/leds_shell/Makefile Outdated Show resolved Hide resolved
examples/leds_shell/Makefile Outdated Show resolved Hide resolved
examples/leds_shell/Makefile Outdated Show resolved Hide resolved
examples/leds_shell/README.md Outdated Show resolved Hide resolved
examples/leds_shell/README.md Outdated Show resolved Hide resolved
examples/leds_shell/README.md Outdated Show resolved Hide resolved
examples/leds_shell/README.md Outdated Show resolved Hide resolved
examples/leds_shell/README.md Show resolved Hide resolved
examples/leds_shell/main.c Show resolved Hide resolved
dylad
dylad reviewed Jul 15, 2024
View reviewed changes
examples/leds_shell/main.c Outdated Show resolved Hide resolved
@krzysztof-cabaj krzysztof-cabaj force-pushed the example-LEDs branch 2 times, most recently from 4075352 to 8d9af20 Compare July 16, 2024 10:24
@krzysztof-cabaj
Copy link
Contributor Author

@mguetschow, @dylad thanks for detailed review.

I hope all issues are now resolved.

mguetschow
mguetschow approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now, thanks again!

@krzysztof-cabaj
Copy link
Contributor Author

@mguetschow thanks for supporting during this PR.

Can we add PR to the merge queue?

dylad
dylad reviewed Jul 17, 2024
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last nitpick on my side, feel free to squash directly.

examples/leds_shell/main.c Outdated Show resolved Hide resolved
@dylad dylad added this pull request to the merge queue Jul 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 17, 2024
@krzysztof-cabaj
Copy link
Contributor Author

krzysztof-cabaj commented Jul 17, 2024 •
edited
Loading

Hmmm ... If I understand CI output well examples/leds_shell did not fit to atmega8 flash.

I add Makefile.ci, but I suspect that more boards with small amount of flash could rise this same error.
Is in RIOT a tool which could I use on my machine to check this automatically? Or can we set some CI flag to not stop after first error?

Update: Maybe labels CI: full build and CI: no fast fail could be used to solve this problem?

@dylad
Copy link
Member

dylad commented Jul 18, 2024

@krzysztof-cabaj you should find the required tools for this job locally under dist/tools/insufficient_memory

@dylad
Copy link
Member

dylad commented Jul 18, 2024

You may also try dist/tools/compile_and_test_for_board/compile_and_test_for_board.py for your board, but to run compile only (no test) for your board. It will try to compile your board for all available examples and tests. A list of all failed compilation should be printed at the end.

@krzysztof-cabaj
Copy link
Contributor Author

I run dist/tools/insufficient_memory/create_makefile.ci.sh and only atmega8 did not fit to flash.

Can we add this PR once again to merge queue?

@mguetschow mguetschow added this pull request to the merge queue Jul 18, 2024
Merged via the queue into RIOT-OS:master with commit 4612cc2 Jul 18, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad left review comments

@mguetschow mguetschow mguetschow approved these changes

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees
No one assigned
Labels
Area: doc Area: Documentation Area: examples Area: Example Applications CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@krzysztof-cabaj @riot-ci @dylad @mguetschow